Expose Command collection properties as mutable IList, remove AddCommand, AddOption and AddArugment methods #1989
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
use a custom type that implements IList to ensure that on every added element the parent is set as well
jozkee
reviewed
Dec 14, 2022
|
|
||
| internal bool HasArguments => _arguments is not null; | ||
|
|
||
| /// <summary> | ||
| /// Represents all of the options for the command, including global options that have been applied to any of the command's ancestors. | ||
| /// </summary> | ||
| public IReadOnlyList<Option> Options => _options is not null ? _options : Array.Empty<Option>(); | ||
| public IList<Option> Options => _options ??= new (this); |
There was a problem hiding this comment.
Suggested change
| public IList<Option> Options => _options ??= new (this); | |
| public IList<Option> Options => _options ??= new(this); |
...ility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt
Show resolved
Hide resolved
| /// <summary> | ||
| /// a wrapper of List<typeparamref name="T"/> that sets parent for every added element | ||
| /// </summary> | ||
| internal sealed class ChildList<T> : IList<T> where T : Symbol |
There was a problem hiding this comment.
Did you consider this?
Suggested change
| internal sealed class ChildList<T> : IList<T> where T : Symbol | |
| internal sealed class ChildList : IList<Symbol> |
There was a problem hiding this comment.
It would be impossible to enforce the right types without perf overhead. Example:
Option<bool> option = new ("--some);
RootCommand root = new ();
// in the line below Arguments would be a list of Symbols, which would allow for adding not just arguments but also options and commands
root.Arguments.Add(option); // no compilation error, at runtime we would later find out that it's not an Argument| @@ -113,26 +82,32 @@ public void AddOption(Option option) | |||
| public void AddGlobalOption(Option option) | |||
There was a problem hiding this comment.
Is weird that now that you are removing AddOption we keep AddGlobalOption. If we follow the pattern this should be named AddGlobal, no? But that sounds weird.
Not asking for a change, just wanted to point that out.
There was a problem hiding this comment.
That is on my TODO list, we just need to find a good name for "global" options. In the near future it might look like this:
Option<bool> help = new ("--help);
help.IsCommon = true;
command.Options.Add(help);# Conflicts: # src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt
|
I believe I've answered all the questions, merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is very similar to #1987, but since adding a new Symbol to Command requires setting a parent I had to implement a custom
IList<T>that sets the parent on every add.